Harden MCP portal authentication changes - #199
Conversation
github-actions
Bot
added
mcp/shared
![ask-bonk[bot]](https://avatars.githubusercontent.com/in/2454580?s=60&v=4){kind=small}
| connected: ServerAuthKind, configured: ServerAuthKind, | ||
| ): boolean { | ||
| return (connected === "token") !== (configured === "token"); | ||
| return configured === "oauth" ? connected === "token" : connected !== configured; |
There was a problem hiding this comment.
The doc-comment above (lines 165-168) is now stale relative to this new logic. It says probing may legitimately move between none and OAuth (bidirectionally), but configured === "oauth" ? connected === "token" : connected !== configured only tolerates that drift when the portal is configured as oauth. When configured as none, a connected oauth state now requires a reconnect (connected !== configured) — the intended new hardening, but it contradicts the comment. Please update the doc to describe the asymmetric rule (an oauth-configured portal may prove public during probing, while an explicitly none-configured portal stays strict).
|
Review submitted with one inline finding. I reviewed the PR (hardening MCP portal authentication). The logic is sound: the auth-mode reconnect rule, the OAuth fallback/callback gates, the connection-generation invalidation, and the per-revision in-flight counter all correctly implement "clear stale OAuth state / refuse paths no longer allowed / invalidate on token-authority change." The truth-table change to I submitted 1 inline finding: the doc-comment on |
972434f to
b4a5027
Compare
2846cdc to
f5c6473
Compare
|
I've completed my review. My analysis confirms the logic is sound across all six files:
The only actionable issue is the stale doc-comment on No new actionable findings. (1 pre-existing finding — the stale |
|
Now let me examine the Consider a portal reconfigured from
Now consider
The logic is coherent. Now let me double check one concern in I've completed a thorough review. The implementation is sound. The only actionable finding is the stale doc-comment on No new actionable findings. (1 pre-existing finding — the stale |
f5c6473 to
a6394d8
Compare
b4a5027 to
0ba0753
Compare
Preview:
|
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
| this.ctx.storage.kv.delete(key); | ||
| } | ||
| this.ctx.storage.kv.put("expiredNotified", false); | ||
| this.log().info("portal repointed", { | ||
| event: "connect.repointed", | ||
| serverHost: hostOf(server.endpoint), | ||
| }); | ||
| if (endpointChanged) { | ||
| this.log().info("portal repointed", { | ||
| event: "connect.repointed", | ||
| serverHost: hostOf(server.endpoint), | ||
| }); | ||
| } | ||
| } |
There was a problem hiding this comment.
🟡 Reconnecting a public portal can leave the account marked as needing a login it never had
The account's recorded sign-in mode is overwritten with the value from configuration (kv.put("server", server) at packages/mcp-shared/src/account.ts:332) before the endpoint has been contacted, so a portal that is actually open to everyone is briefly — or, if the contact attempt fails, permanently — recorded as requiring a login it has no credentials for.
Impact: While a user reconnects an open portal, in-flight tool calls fail with "this connection is not authorized" and the user is told their credentials expired; if the reconnect attempt errors out, the account stays broken until a later reconnect succeeds.
Auth-only change persists an unverified auth mode before the probe
Before this PR only endpointChanged caused the target record to be written ahead of the probe. authChanged (packages/mcp-shared/src/account.ts:330) now does the same. For the portal connector the target always carries the configured auth kind, while the stored record carries the observed one: a portal configured oauth whose endpoint answered publicly is stored with auth: "none" (packages/mcp-shared/src/account.ts:377-379). Every reconnect of such an account therefore sees existing.auth === "none" !== "oauth" and takes the branch, writing {...auth: "oauth"} and deleting tokens/oauthClient/oauthDiscovery before probe() runs.
During that await any facet call reaches #getAuthorization with server.auth === "oauth" and no stored tokens, which calls noteCredentialsExpired and throws "This MCP connection is not authorized" (packages/mcp-shared/src/account.ts:744-749). The portal's own getConnection guard does not stop it: portalAuthRequiresReconnect("oauth", "oauth") is false.
If the probe throws anything other than McpAuthRequiredError (transient network failure), restoreSelection re-opens the nonce but the stored server record is left at auth: "oauth", so the previously working public-portal account fails every subsequent call until a reconnect completes successfully.
(Refers to lines 330-345)
Prompt for agents
In McpAccountBase.beginConnect (packages/mcp-shared/src/account.ts), the new `authChanged` condition now persists the target ConnectedServer (including its configured `auth`) and wipes OAuth state before the endpoint has been probed. The stored record holds the *observed* auth mode (a portal configured `oauth` that answered publicly is stored as `none`, see the `connected` assignment later in the same method), while the target holds the *configured* mode, so `authChanged` is true on every reconnect of a public-but-oauth-configured portal. Two consequences: (1) during the probe await, concurrent facet calls hit `#getAuthorization` with `auth: "oauth"` and no tokens, which fires `noteCredentialsExpired` and throws "not authorized"; (2) if the probe fails with a non-auth error, the account is left recorded as `oauth` with no credentials, breaking a previously working account. Consider keeping the pre-probe `kv.put("server", ...)` limited to an endpoint repoint (the case that must fail closed for static tokens), and for an auth-only change either clear stale credential state without rewriting the recorded auth mode, or restrict the branch to transitions that actually change credential authority (to/from `token`, or oauth->none) rather than the `none`->`oauth` guess.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
Portal authentication mode and deployment-owned tokens can change while an account or facet remains live. This makes auth-mode transitions clear stale OAuth state, refuses fallback/callback paths no longer allowed by current configuration, and invalidates captured connection generations when token authority changes.
OAuth redirects record their configuration revision before handing off to the callback, preventing an existing facet from invalidating the attempt while the user is authorizing.
Stacked on #170. Verified with the MCP and portal suites plus full
pnpm lint.